Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom comparison for Montrose::{Schedule/Recurrence} #148

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thewatts
Copy link
Contributor

@thewatts thewatts commented Oct 22, 2021

In our application using Montrose, we have some instances where we want
to be able to check to see if a schedule has changed.

The specific use case for us is with a Rails model that is serializing a
schedule - we want to be able to know if the schedule itself has
changed, ie: event.schedule_changed? #=> false

The same attributes may be assigned for the schedule's configuration -
but Rails thinks that the schedule has changed b/c it's comparing
the objects themselves instead of the underlying recurrence
configurations.

This commit overloads Montrose::Schedule#== so that schedules can be
compared against each other within the context of their configurations.

Edit:

In addition, I added the same comparison for Montrose::Recurrence

In our application using Montrose, we have some instances where we want
to be able to check to see if a schedule has changed.

The specific use case for us is with a Rails model that is serializing a
schedule - we want to be able to know if the schedule itself has
changed, ie: `event.schedule_changed? #=> false`

The same attributes may be assigned for the schedule's configuration -
but Rails thinks that the schedule has changed b/c it's comparing
the objects themselves instead of the underlying recurrence
configurations.

This commit overloads `Montrose::Schedule#==` so that schedules can be
compared against each other within the context of their configurations.
@thewatts thewatts changed the title Add custom comparison for Montrose::Schedule Add custom comparison for Montrose::{Schedule/Recurrence} Oct 24, 2021
In the same vein as adding `Montrose::Schedule#==`

This allows `Montrose::Recurrence` objects to be compared by their
underlying hash options.
@thewatts
Copy link
Contributor Author

Something to note here:

In each case, I'm allowing someone to pass in an Array (for Schedule) or Hash (for Recurrence).

I started writing specific spec cases for this, but I couldn't get minitest to operate how I would have expected it to.

schedule = new_schedule.tap do |schedule|
  schedule << {every: :month}
  schedule << {every: :day}
end

identical_array = [
  {every: :month},
  {every: :day}
]

schedule == identical_array # => true
assert_equal schedule, identical_array # => true

_(schedule).must_equal identical_array # => 💣 

I'd love to add specs here - but I'm not quite sure why .must_equal is having this effect, as I've (perhaps wrongly) assumed that assert_equal and must_equal are identical:

https://github.com/seattlerb/minitest/blob/3c6576a51f4e266996e3459d7a0dd054eb4c87f7/lib/minitest/expectations.rb#L38

@rossta
Copy link
Owner

rossta commented Oct 28, 2021

Thank you for the PR! I like the idea of implementing #== for Schedule and Recurrence.

I don't think we will equate with Array and Hash. Callers can still cast accordingly, e.g., schedule == Schedule.new(array) and recurrence == Recurrence.new(hash).

As for the Schedule implementation, how about we delegate to Recurrence#==, something like this:

def ==(other)
  if other.is_a?(self.class)
    all?(&:==)
  else
    super
  end
end

@saturnflyer
Copy link

saturnflyer commented Jan 14, 2024

Just offering some drive-by feedback on this while I'm evaluating using montrose in my project.

A coercion method would be useful here to convert the other object so that you can depend on the methods defined there.

def ==(other)
  Recurrence(other).all?(&:==)
end

That method might look like:

def Recurrence(object)
  if object.is_a?(Recurrence)
    return object
  else
    Montrose.recurrence(object)
  end
end

The same could be done for Schedule. I'm not sure if that implementation is exactly correct for this library but it would function like the Array() method.

EDIT: Here's how I'm patching this with an initializer

require "montrose"

module RecurrenceComparision
  def ==(other)
    Montrose.recurrence(other).to_hash == to_hash
  end
end
Montrose::Recurrence.prepend(RecurrenceComparision)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants